Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: initial support for case type selection #2208

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

espoal
Copy link

@espoal espoal commented Jul 31, 2023

Summary

This is the second of a multi PR effort aiming at supporting custom name casing conventions, as detailed here.

  1. The first PR attempts to update @nestjs/schematics to accept a caseType option when generating a new file.
  2. In this PR we add a caseNaming field to GenerateOptions
  3. This is the PR to document the new option
  4. Future PRs will extend this feature in term of supported actions, tests and configuration

For now I decided to support only controllers, to reduce the changes size and make reviewing easier. If the approach will be approved I will make sure to extend it to other objects and to add more tests.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Currently @nestjs/cli uses kebab-or-snake case, as mentioned in this issue.

Issue Number: 462

What is the new behavior?

The goal is to add a caseNaming option to nest-cli.json so that generated files will follow the desired name casing convention.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other informations

How to run

  • Install deps and build:
npm install
npm run build
  • Build updated @nestjs/schematics
    Clone this branch, and do:
npm install
npm run build
  • Install the updated @nestjs/schematics package
    Copy the dist folder in @nestjs/nest-cli deps: nest-cli/node_modules/@nestjs/schematics/dist/

  • Run it with one of those commands:
    node bin/nest.js new test-nest -s --caseNaming=camel to generate a custom nest-cli.json
    node bin/nest.js g co keb-pap in a folder with the correct nest-cli.json

@espoal espoal changed the title feat: initial support for case naming feat: initial support for case type selection Jul 31, 2023
@espoal espoal marked this pull request as ready for review August 15, 2023 13:45
@espoal
Copy link
Author

espoal commented Aug 15, 2023

Hey @micalevisk can I help you with anything? I would love a pair programming session to review the pr together and see how to improve it

@mwanago
Copy link

mwanago commented Aug 15, 2023

@espoal
I think it would make sense to add the --caseNaming option to the new command that creates a new NestJS application. Would you agree?

@micalevisk
Copy link
Member

micalevisk commented Aug 15, 2023

yeah, instead of adding that new option to generate command, we can move it to new and to the configuration file (nest-cli.json)

@mwanago
Copy link

mwanago commented Aug 15, 2023

@micalevisk
Since we usually want to keep the same naming convention throughout the project, storing this option in nest-cli.json sounds like a great idea

@espoal
Copy link
Author

espoal commented Aug 17, 2023

@mwanago Let's split the problem in two parts:

  • My main concern is if my approach of decorating SchemaOption is a good idea, or if maybe people see a better approach.
  • I would totally be ok to use the new command. Could you make an example of how a CLI command would be using that? And where the nest-cli.json is parsed?

@micalevisk
Copy link
Member

micalevisk commented Aug 27, 2023

@espoal

I would totally be ok to use the new command. Could you make an example of how a CLI command would be using that?

it would be nothing more than a new option (--caseNaming <option>) for the new command that will define the value of the caseNaming option (defaults to "kebab") of the generated nest-cli.json (I guess at GenerateOptions interface here: https://github.com/nestjs/nest-cli/blob/48939b57814f0353e1759fcabb85fe401b39e793/lib/configuration/configuration.ts#L76C18-L76C33)

Then, when using the generate command, that option will be taken into account just like it does for the spec option which uses the value from nest-cli.json.

In this case, we won't have --caseNaming for others commands.

@espoal
Copy link
Author

espoal commented Sep 29, 2023

Sorry for the late reply but I was swamped with my day work.
The PR is far from perfect, but I think I'm slowly getting closer to what you meant @micalevisk .
Some questions:

  • I still cannot change the suffix of the file (KebPap.controller.ts instead of KebPapController.ts). Any advice on how to proceed?
  • Now I correctly use the nest-cli.json but are we sure we don't want to leave the command line option? Now we have both.
  • I cannot see why the pipeline is failing. Locally it seems to work. Suggestions on how to fix it?

And a slightly bigger topic:

  • Now when we use the new command the file don't respect the passed convention, because they're taken from a template. The solution IMHO would be to update the new command to use a sequence of commands (configuration, generate ....) instead of the templates. But this seems like a big topic, so maybe you have suggestions.

cc: @mwanago

package.json Outdated Show resolved Hide resolved
@espoal
Copy link
Author

espoal commented Oct 5, 2023

if we can make it work without adding the options to the generate command, then yes. Less code to maintain; less options to know about. We can add them in the future if needed.

I think it's not too much code, but I also think that smaller PRs are better. Let's keep the flag for a future PR.

The project generated by new is too simple.

That's a fair point

Also, convert this PR to draft until you're done.

I still don't think the PR is perfect, but perfection is also the worst enemy of the good, so I would like to ask you @micalevisk to give another look.

Things I would like to do after this review:

  • Decide which case formats we support
  • Add documentation
  • Add more tests

P.S.: I managed to fix the pipeline 🥳

test/lib/utils/formatting.spec.ts Outdated Show resolved Hide resolved
actions/generate.action.ts Outdated Show resolved Hide resolved
@espoal espoal marked this pull request as ready for review October 16, 2023 08:05
{
description: 'should manage classified string option value name',
option: 'name',
input: 'MyApp',
expected: 'my-app',
},
{
/*{
description: 'should manage parenthesis string option value name',
Copy link
Author

@espoal espoal Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Atm we don't really respect this. In package.json we have the right name my-(app), but at least on linux the folder is actually called ./'my-(app)' (note the dashes). Same for the [ character.
The test fails because the case conversion function performs sanitization, but I could fix it with this:

    case 'kebab':
      return kebabCase(str, { keep: ['[', ']', '(', ')'] })

@@ -38,19 +38,19 @@ describe('Schematic Option', () => {
input: 'myApp',
expected: 'my-app',
},
{
/*{
description: 'should allow underscore string option value name',
Copy link
Author

@espoal espoal Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this could be a source of confusion: if I specify the kebab case, I would expect the convention to be respected.
The code atm reflect this behavior, hence the test fails because my_app turns to my-app.

@espoal
Copy link
Author

espoal commented Oct 17, 2023

In the end I decided that the case we should support are:

  • snake
  • kebab
  • camel
  • pascal

I would propose to drop the kebab-or-snake case for the reasons outlined here. Let me know if you would prefer to keep it for now and remove it in a future PR.

@micalevisk I would love to ask you for another review

@espoal
Copy link
Author

espoal commented Nov 6, 2023

Hey @micalevisk is there anything I could do to speed up the merging of this PR?

@micalevisk
Copy link
Member

@espoal nop. I'll try to review this ASAP

@espoal
Copy link
Author

espoal commented Nov 6, 2023

Thanks, I apologies to bother you but I'm really excited about my first nestjs PR ❤️

}
};

export const formatString = (str: string) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you elaborate here in the code on what this function is supposed to do?

github copilot can help :p

Copy link
Author

@espoal espoal Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang, I'm surprised copilot understood immediately the code. I'm starting to question reality :D

Added some JsDoc. Basically this function escapes parenthesis which will then later be removed.

I see here an opportunity to create @nestjs/utils or @nestjs/stringUtils because this code is shared across projects, and I feel it would be beneficial to centralize it so that it's easier to maintain and can be reused by plugin developers. I didn't do it to keep the scope limited, but it shouldn't be too hard to do and I would be up for it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I updated the same code also in @nestjs/schematics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants